-
Notifications
You must be signed in to change notification settings - Fork 35
Dockerfile improvements #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Pin Alpine and Node versions - Use Alpine for final image (smaller) - Cache node modules in a Docker layer for build - Run without root as user with uid 1000
|
If I am correct I believe we shifted the react Dockerfile to use Nginx. I’d be interested in a Go binary though because as of right now Nginx output takes up the entire terminal window when people connect to the site making it hard to see other output.
|
Cool, I'll write an http server this weekend to serve this react code. If you don't mind, I'll add some features to modify config.json as well so it can replace the entrypoint.sh at the same time. With such server, we can then easily cross CPU compile it so it works on all CPU architectures supported by Docker as well (arm, 386, s390x etc.). |
|
One small concern but worth pointing out - the swd command will run every time the container is started, but if the env var is changed for whatever reason then it will fail to update the config file on subsequent runs, and would require destroying the container entirely. |
|
@Woodham we can copy the files to a temp directory and modify them there at every start, and clear the directory at start or shutdown. Or use a regex so that it replaces the entire line for example. I think the temp directory may be better for ease of modifications. |
|
@Woodham would mounting the config file fix this issue? Im not sure running the |
|
Why not offer both solutions 😉 We could have the original source in
Are you talking about ten parameters or more a thousand? |
closer to 10 than 1000 |
|
If you'd prefer to stick to env vars (and I totally understand why) you could create a separate config.json.template file and don't do the sed replacement in-line. |
|
Yeah I think we want to avoid a |
|
Personally I'd go against letting the user manage files, as it's prone to error vs environment variables. For example: windows vs Unix line endings, permission issues, bad formatting of JSON, more tedious debugging. It's also a few extra steps to get it working. Anyway let me know what you think guys, I'll get started on an http server entrypoint soon. |
|
Hey @qdm12 - I had a bit of a think about this and I think the best way to get this going for now is to do something similar to the way the nginx dockerfile approaches this problem. It uses a utility called I've opened a PR (to your PR branch) with an implementation of this if you'd like to take a look (plus a couple of other changes). - https://github.com/qdm12/Lightspeed-react/pull/1 |
* Change final base image to nginx * use envsubst to replace environment variables in config file * create config file template * move docker specific files to directory
|
@Woodham PR is merged into mine 👍 Thanks! I think this is good, everyone's happy for now? I'll add documentation if so. I'll do a Go binary entrypoint / scratch based image in a subsequent PR. |
|
Hopefully it all makes sense to you @GRVYDEV. I think this should be enough to allow people to easily deploy an instance :) |
Yeah Im not sure this makes much sense to me right now. Here is the solution I came up with for docker compose. I guess the only downside to my solution is that it doesn't work outside of compose. Im a bit confused at how this solution works. So at runtime the |
Yep you've got it exactly - it uses a command line tool called I actually agree that the mounting the config file like you've done there is the simplest solution (and can definitely work outside of compose btw - you can mount files like that using the normal docker run command too) - the benefit here primarily that env vars can be used instead. I think which way to go is purely preference at this point. This specific implementation currently takes advantage of the fact that the nginx docker image will run any |
It's also about abstraction. Having as environment variables allows the underlying implementation (nginx, react's config.json) to change. And implementations do change over time, so it might be a good idea not to rely on the user bind mounting a config.json file. |
|
I think we should go forward and merge this with that environment variable substitution. I would like to contribute with a Go http server & env variables "entrypoint" program but that's a bit blocked by this PR first. |
|
@GRVYDEV also are we planning on having this frontend served as a microservice (have its own image part of a docker-compose/k8s)? If so I'd need you to:
I can set it up to build and push to Docker Hub using Github Actions for all cpu architectures (raspberry pis and so on). |
|
|
||
| ```sh | ||
| docker build -t grvydev/lightspeed-react https://github.com/GRVYDEV/Lightspeed-react.git | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that steps 1 and 2 will be removed once the image is built and pushed by the CI to Docker Hub
|
Here is my WIP .env docker-compose.yaml I'd personally prefer to not have to use host network and specify ports explicitly. But I don't have it working because I don't understand the required tcp/udp ports yet. I'm not getting any errors or logs, OBS says it's streaming. The For building locally we can do Git Submodules or the alternative:
I have a slight hesitation in using Git Submodules because that might make it harder/tedious for the developers who need to checkout and work on different branches. I honestly haven't implemented submodules before so I could be wrong, just keep that in mind. |
Ideally maybe we can keep it to separate repos and the user can just build: https://github.com/GRVYDEV/Lightspeed-react.gitFor each image, so the user/dev can just |
GRVYDEV
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Shell entrypoint to run sed command using the environment variable
WEBSOCKET_HOSTand defaulting to localhost if it's not set. It then executes /bin/sh.Faster Docker builds by caching node modules in a Docker layer for build
Pin Alpine and Node versions
Smaller final Docker image with alpine
Run without root as user with uid 1000, for security reasons 👀- will do with the Go entrypoint due to how nginx operates❓ We should have an http server in the Dockerfile, do you have any preference? Maybe nginx or a Go binary would be nice to keep the size lower than a node image with serve. I'm happy to hack one with Go, or you can use my ugly 1 line one (lol) from this
Add documentation once we're all happy